Invoke StopAll() in terminal exit pathways when service startup fails#68
Conversation
bmcdonnel
force-pushed
the
stop-services-on-startup-failure
branch
4 times, most recently
from
7682bff to
a5d60b0
Compare
dzbarsky
left a comment
dzbarsky
left a comment
There was a problem hiding this comment.
the motivation makes sense! can you please take a look at how this interact with ibazel reload loop? that's the only bit I'm worried about :)
| mustStopAll() | ||
| return | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
nit: combine with the above? like
if err != nil {
mustStopAll()
if errors.Is(err, context.Canceled) {
return
}
}
There was a problem hiding this comment.
updated! much cleaner
| r, err := runner.New(ctx, serviceSpecs) | ||
| must(err) | ||
|
|
||
| mustStopAll := sync.OnceValue(func() map[string]*os.ProcessState { |
There was a problem hiding this comment.
mid-review note: how does this work with reload? Is there any way that services can get restarted after this is called (and then this becomes no-op in future?)
There was a problem hiding this comment.
renamed to mustStopAllForExit to signal that it is intended for final shutdown pathways, not reload pathways.
TODO(zbarsky): what is the right behavior here when services are crashing in ibazel mode?
I saw this TODO and wasn't sure what the right approach would even be for reload failure cleanup. open to suggestions if we should try to address that in this PR! 🙏
There was a problem hiding this comment.
oh ok if it's not getting called during reload this should be no worse than before!
bmcdonnel
force-pushed
the
stop-services-on-startup-failure
branch
2 times, most recently
from
15c0298 to
9c28ff4
Compare
bmcdonnel
force-pushed
the
stop-services-on-startup-failure
branch
from
9c28ff4 to
3b55cb0
Compare
|
|
||
| cleanup_failure_test( | ||
| name = "cleanup_on_startup_failure_test", | ||
| target_compatible_with = NOT_WINDOWS, |
There was a problem hiding this comment.
marked this test as NOT_WINDOWS due to .sh script needed for testing failure cleanup behavior. but apparently, runner/pgroup_windows.go uses cmd.Process.Kill() which (i'm told) isn't really trappable anyway? So maybe this test isn't really doable on Windows ...
There was a problem hiding this comment.
yeah i think that's ok, windows is very best-effort
bmcdonnel
changed the title
|
@dzbarsky, let me know if you need any other evidence or changes on this PR. would love to get this merged v soon! thanks so much 🙏 |
sorry, i could have sworn i hit the merge button when posting those comments. i'll cut a release now |
2 participants
Refs #67.
Summary
mustStopAllForExitfunc which invokesr.StopAll()mustStopAllForExit()in terminal exit pathways only:cleanup_on_startup_failure_testasNOT_WINDOWSdue to.shscript needed to test failure cleanup behaviorValidation
cd tests && bazel test --cache_test_results=no --test_output=errors //startup_failure:cleanup_on_startup_failure_test //startup_failure:service_exits_before_healthy_failurecd tests && bazel test --cache_test_results=no --test_output=errors //...Screenshot of new test case output
See
cleanup service received shutdown.